gh-137838: Move _PyUOpInstruction buffer to PyInterpreterState #138918

corona10 · 2025-09-15T12:15:01Z

Issue: Make no-opt JIT to pass whole unit test. #137838

…to _PyThreadStateImpl

corona10 · 2025-09-15T12:15:45Z

Include/internal/pycore_tstate.h

+ *    uint16_t jump_target;
+ *    uint16_t error_target;
+ */
+typedef struct _PyUOpInstruction{


@Fidget-Spinner I move _PyUOpInstruction to here becasue of declaration issue. But we may need to move this struct for better place.

corona10 · 2025-09-15T12:17:15Z

I plan to add a new CI for nopt JIT with a separate PR.

corona10 · 2025-09-15T12:19:17Z

And I fully checked that PYTHON_UOPS_OPTIMIZE=0 ./python.exe -m test -j8 is passed on my local.

Misc/NEWS.d/next/Core_and_Builtins/2025-09-15-13-14-20.gh-issue-137838.Ju-vRW.rst

Fidget-Spinner

Looks good, but just come over and ask Mark if he's okay with the struct being in pycore_optimizer.h.

corona10 · 2025-09-15T12:47:00Z

he's okay with the struct being in pycore_optimizer.h.

Now it's currently located in pycore_tstate.h

Fidget-Spinner · 2025-09-15T12:47:44Z

he's okay with the struct being in pycore_optimizer.h.

Now it's currently located in pycore_tstate.h

Woops yes that's the one I was thinking of. Thanks.

corona10 · 2025-09-15T12:49:29Z

Do you feel comfortable if I define pycore_uop.h?

Fidget-Spinner · 2025-09-15T12:50:17Z

Do you feel comfortable if I define pycore_uop.h?

That works too.

This reverts commit ab39a55.

This reverts commit 877f3a9.

corona10 · 2025-09-17T05:36:39Z

38 tests failed again:
    test.test_asyncio.test_base_events
    test.test_asyncio.test_buffered_proto
    test.test_asyncio.test_context
    test.test_asyncio.test_eager_task_factory
    test.test_asyncio.test_events
    test.test_asyncio.test_free_threading
    test.test_asyncio.test_futures test.test_asyncio.test_futures2
    test.test_asyncio.test_graph test.test_asyncio.test_locks
    test.test_asyncio.test_pep492
    test.test_asyncio.test_proactor_events
    test.test_asyncio.test_queues test.test_asyncio.test_runners
    test.test_asyncio.test_selector_events
    test.test_asyncio.test_sendfile test.test_asyncio.test_server
    test.test_asyncio.test_sock_lowlevel test.test_asyncio.test_ssl
    test.test_asyncio.test_sslproto test.test_asyncio.test_staggered
    test.test_asyncio.test_streams test.test_asyncio.test_subprocess
    test.test_asyncio.test_taskgroups test.test_asyncio.test_tasks
    test.test_asyncio.test_threads test.test_asyncio.test_timeouts
    test.test_asyncio.test_waitfor
    test.test_asyncio.test_windows_events
    test.test_concurrent_futures.test_interpreter_pool
    test.test_inspect.test_inspect test_asyncgen test_coroutines
    test_external_inspection test_logging test_pdb test_regrtest
    test_unittest

When I move to interpreter state, those tests are failed and could be race condition in some where, need to investgate with Windows machine.

corona10 · 2025-09-17T06:28:49Z

I still need to debug this in detail, but the buffer causes problems when it’s shared at the interpreter level under an asyncio workload. A per-thread approach also seems safe for the GIL build.
Before this PR, it was safe because it was allocated in stack-wise while the optimizer is needed.

Python/pystate.c

corona10 · 2025-09-17T09:12:00Z

(3460.15a4): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
_asyncio_d!add_tasks_llist+0x20:
00007ffe`eb88fe50 488b00          mov     rax,qword ptr [rax] ds:00000000`00000000=????????????????

corona10 · 2025-09-17T09:15:44Z

Okay it turn out that it is not related to this change but looks like trigger other bug in asyncio. Let me test..

This reverts commit 989fa8e.

corona10 · 2025-09-17T10:25:53Z

@brandtbucher @Fidget-Spinner Fixed! Ready to be merged!

corona10 · 2025-09-17T10:28:03Z

Include/internal/pycore_interp_structs.h

    struct _stoptheworld_state stoptheworld;
    struct _qsbr_shared qsbr;

+    struct _PyUOpInstruction *jit_uop_buffer;


The issue was that I declared this field only for the TIER2 build.
We might need to adjust it properly if we want to exclude non-JIT builds.
But since the field is only allocated when the TIER2 build is enabled, I think it’s fine to leave it as is.

markshannon · 2025-09-17T10:54:27Z

Thanks for doing this.

I have a few comments:

If we define UOP_MAX_TRACE_LENGTH in terms of the size of the buffer, we won't leak the optimizer details to the thread state.
i.e.

#define UOP_MAX_TRACE_LENGTH JIT_TRACE_BUFFER_SIZE/sizeof(_PyUOpInstruction)

It might be better to use _PyObject_VirtualAlloc so that we can over allocate without wasting physical memory.
Can we add asserts that the optimizer doesn't re-enter so that we don't overwrite the buffer while we are using it?

This reverts commit a49bcab.

corona10 · 2025-09-17T14:06:29Z

Python/pystate.c

 #ifdef _Py_TIER2
    _Py_ClearExecutorDeletionList(interp);
+    if (interp->jit_uop_buffer != NULL) {
+        _PyObject_VirtualFree(interp->jit_uop_buffer, UOP_BUFFER_SIZE);


@brandtbucher _PyObject_VirtualFree doesn’t return a value, and the current CPython codebase doesn’t check whether it succeeds or not

corona10 · 2025-09-17T14:13:39Z

@markshannon Hopefully you will like the current change :) I 've applied most of the changes you requested!

markshannon

Looks good now.
Thanks

pythongh-137838: Move _PyUOpInstruction buffer[UOP_MAX_TRACE_LENGTH] …

fe3c1af

…to _PyThreadStateImpl

corona10 requested review from ericsnowcurrently and markshannon as code owners September 15, 2025 12:15

corona10 requested review from Fidget-Spinner and removed request for ericsnowcurrently and markshannon September 15, 2025 12:15

bedevere-app bot added the awaiting core review label Sep 15, 2025

corona10 requested a review from brandtbucher September 15, 2025 12:15

bedevere-app bot mentioned this pull request Sep 15, 2025

Make no-opt JIT to pass whole unit test. #137838

Closed

corona10 commented Sep 15, 2025

View reviewed changes

nit

cd868b9

corona10 changed the title ~~gh-137838: Move _PyUOpInstruction buffer[UOP_MAX_TRACE_LENGTH] to _Py…~~ gh-137838: Move _PyUOpInstruction buffer to _PyThreadStateImpl Sep 15, 2025

nit

daf4730

corona10 added the topic-JIT label Sep 15, 2025

Fidget-Spinner reviewed Sep 15, 2025

View reviewed changes

Misc/NEWS.d/next/Core_and_Builtins/2025-09-15-13-14-20.gh-issue-137838.Ju-vRW.rst Outdated Show resolved Hide resolved

corona10 added the skip news label Sep 15, 2025

Update

e09af56

Fidget-Spinner approved these changes Sep 15, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Sep 15, 2025

Address code review

be0b25b

corona10 requested review from a team, erlend-aasland and AA-Turner as code owners September 15, 2025 12:59

corona10 added 2 commits September 16, 2025 23:27

Revert "test"

60541dc

This reverts commit ab39a55.

Just for test

877f3a9

corona10 added the DO-NOT-MERGE label Sep 16, 2025

Revert "Just for test"

9af48c2

This reverts commit 877f3a9.

Test statically allocated

989fa8e

Fidget-Spinner reviewed Sep 17, 2025

View reviewed changes

Python/pystate.c Outdated Show resolved Hide resolved

Python/pystate.c Show resolved Hide resolved

corona10 added 4 commits September 17, 2025 10:16

Revert "Test statically allocated"

4dff816

This reverts commit 989fa8e.

Make situation to isolate with the change

7e5261c

fix

08adf26

fix

9051791

corona10 removed the DO-NOT-MERGE label Sep 17, 2025

corona10 commented Sep 17, 2025

View reviewed changes

corona10 changed the title ~~gh-137838: Move _PyUOpInstruction buffer to _PyThreadStateImpl~~ gh-137838: Move _PyUOpInstruction buffer to PyInterpreterState Sep 17, 2025

corona10 added 5 commits September 17, 2025 13:36

Address code review

928485a

nit

0c2a74f

Update

a49bcab

Revert "Update"

f88ee3b

This reverts commit a49bcab.

fix

05825f5

corona10 commented Sep 17, 2025

View reviewed changes

markshannon approved these changes Sep 17, 2025

View reviewed changes

corona10 merged commit d873fb4 into python:main Sep 17, 2025
58 checks passed

bedevere-app bot removed the awaiting merge label Sep 17, 2025

corona10 deleted the gh-137838 branch September 18, 2025 17:20

Uh oh!

gh-137838: Move _PyUOpInstruction buffer to PyInterpreterState #138918

gh-137838: Move _PyUOpInstruction buffer to PyInterpreterState #138918

Uh oh!

Conversation

corona10 commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

corona10 commented Sep 15, 2025

Uh oh!

corona10 commented Sep 15, 2025

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 commented Sep 15, 2025

Uh oh!

Fidget-Spinner commented Sep 15, 2025

Uh oh!

corona10 commented Sep 15, 2025

Uh oh!

Fidget-Spinner commented Sep 15, 2025

Uh oh!

corona10 commented Sep 17, 2025

Uh oh!

corona10 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 commented Sep 17, 2025

Uh oh!

corona10 commented Sep 17, 2025

Uh oh!

corona10 commented Sep 17, 2025

Uh oh!

corona10 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

markshannon commented Sep 17, 2025

Uh oh!

corona10 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

corona10 commented Sep 17, 2025

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

corona10 commented Sep 15, 2025 •

edited

Loading

corona10 commented Sep 17, 2025 •

edited

Loading